Initialization Module - #3912
Conversation
Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
|
I think I am done making changes. This should be ready for a re-review. If I forgot something or did not address a comment appropriately, please let me know. |
|
And I already lied. I forgot one change. Should be good for a re-review now. |
mrmundt
left a comment
There was a problem hiding this comment.
I do have somewhat stronger feelings about model vs nlp on the public-facing functions. Otherwise, this is looking great.
| # stat, x = lp_init_ex() | ||
| # stat, x = pwl_init_ex() |
There was a problem hiding this comment.
Intentionally commented out?
There was a problem hiding this comment.
Yes. This was intentional. Just to make it easy to run the example with different methods.
| logger.info( | ||
| f'solved NLP with {nlp_solver.name}: {res.solution_status}, {res.termination_condition}' | ||
| ) | ||
| if res.solution_status in {SolutionStatus.feasible, SolutionStatus.optimal}: |
There was a problem hiding this comment.
I agree with Michael - this can be in a separate PR.
|
|
||
|
|
||
| def initialize_with_piecewise_linear_approximation( | ||
| nlp: BlockData, |
There was a problem hiding this comment.
John asked a similar question on I think a test file earlier ("why nlp instead of model?"). For tests, I really don't think it matters, but here, I think it should be model. It'll make more sense to our users if they look at the docs to see model (the Pyomo style standard) rather than nlp.
There was a problem hiding this comment.
I didn't realize there was a pyomo style standard here. I don't think I agree that model is any more descriptive than nlp. Also, we are not consistent at all. In fact, I think we use m more than model:
find ./ -name "*.py" | xargs grep "(m," | wc -l: 1685
find ./ -name "*.py" | xargs grep "(model," | wc -l: 1025
find ./ -name "*.py" | xargs grep "(instance," | wc -l: 104
find ./ -name "*.py" | xargs grep "(block," | wc -l: 126
find ./ -name "*.py" | xargs grep "(b," | wc -l: 246
There was a problem hiding this comment.
There are also many other examples, such as working_blk, working_model, separation_model, subproblem, root, blk, scenario_instance, ef, etc. Some of these argument names have meanings that are significantly more useful than m or model.
|
|
||
|
|
||
| def initialize_with_LP_approximation( | ||
| nlp: BlockData, |
There was a problem hiding this comment.
Same as above (model instead of nlp).
|
|
||
|
|
||
| def initialize_with_global_opt( | ||
| nlp: BlockData, |
There was a problem hiding this comment.
Same as above (model instead of nlp).
Co-authored-by: Miranda Mundt <55767766+mrmundt@users.noreply.github.com>
jsiirola
left a comment
There was a problem hiding this comment.
Two minor changes / questions (and some notes for later development)
| return opt | ||
|
|
||
|
|
||
| def _setup(nlp): |
There was a problem hiding this comment.
Should setup also relax any discrete variables to continuous domains?
There was a problem hiding this comment.
I added an exception for now. I want some time to think about what it would mean to allow models with discrete variables.
| ) | ||
|
|
||
|
|
||
| def shallow_clone(m1): |
There was a problem hiding this comment.
In the future, we should promote this to a standard transformation. Not only does it make a fast shallow copy, but it collapses the entire block structure, which could be useful in a lot of contexts. It could generate more of a "flattened view" into a model than a "shallow copy"?
There was a problem hiding this comment.
That is a great suggestion. I don't think "view" is the right term. What you get is a valid model. Flattened is very accurate, though...
There was a problem hiding this comment.
Would this fit into the normal transformation workflow? apply_to wouldn't really make sense, and create_using might also not do what you expect. Maybe I'm thinkin about it wrong...
There was a problem hiding this comment.
My initial thought would be to have apply_to raise an exception and only support create_using (similar to the dual transform). I would argue that "view" is correct, as when you solve the flattened view, the solution "appears" in the original structured model (because they share variables). If we go down the transformation route, there are questions around other components, though (e.g., should we map results for suffixes back?).
There was a problem hiding this comment.
That is a good point. I thought that there was an expectation of create_using that a full clone would get created?
There was a problem hiding this comment.
I guess that is where naming and documentation comes in.
There was a problem hiding this comment.
The idea was apply_to is "in-place" and create_using is "out-of-place". There was concern that "in place" and "out of place" were less user friendly, and apply_to and create_using (short for "create a new model using data from model m") were more clear.
"Cloning" only came into play because many transformations (like GDP) are most efficiently written in-place, so the default implementation of create_using is simply to clone and then run apply_to...
There was a problem hiding this comment.
Got it. That makes sense.
|
One of the GHA jobs is randomly getting stuck so I'm going to merge this without waiting for the last check. |
Fixes #3878
Summary/Motivation:
This PR adds a module to
develcalledinitialization. The goal of the module is to provide methods to help initialize nonconvex nonlinear programming problems.Changes proposed in this PR:
initializationmoduleLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: